[Saved Objects] Update the `migrationVersion` property to hold a plain string value #150075

dokmic · 2023-02-01T16:13:37Z

Summary

Resolves #70815.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…tring value

…s a string value

…ionVersion`

…nstead of deprecated `migrationVersion`

…grationVersion` property

elasticmachine · 2023-03-20T12:57:37Z

Pinging @elastic/kibana-core (Team:Core)

vadimkibana

I see SharedUX is tagged for review because of src/plugins/home/server/services/sample_data, changes in sample LGTM.

nreese

kibana-gis changes LGTM

jgowdyelastic

ML changes LGTM

rudolf

Just did a first pass and will go through the code in more detail again.

We're technically making a breaking change to the API by not responding with migrationVersion anymore. It's hard to know if external integratinos ever rely on this field but we should rather be safe than sorry.

I think we can add a very light-weight compatibility layer by basically applying the transformation from packages/core/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/migrations/transform_migration_version.ts (and it's opposite) at the API layer. That way the API stays 100% the same.

That means we can remove migrationVersion in all the rest of our code like the documentMigrator and saved objects typescript APIs as well.

...re/saved-objects/core-saved-objects-migration-server-internal/src/document_migrator/types.ts

rudolf · 2023-03-22T13:01:07Z

...saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts

@@ -159,6 +159,9 @@ export function getBaseMappings(): IndexMapping {
      coreMigrationVersion: {
        type: 'keyword',
      },
+      typeMigrationVersion: {


can't comment at the right place, but we should remove the mapping for migrationVersion on line 124

We cannot remove that yet as we are updating mappings when there are still documents with the old migrationVersion properties. That happens before we apply core migrations.

Removing a field will mean the migration algorithm doesn't think we've made an incompatible mappings change, so it would re-use the index instead of creating a new one. But mappings updates are commutative, so even if we update the mappings without migrationVersion ES will still keep it on existing indices.

Whenever we create a new index we should run the OUTDATED_DOCUMENTS_TRANSFORM* steps before we apply the final mappings to the index.

So I can't really imagine where this would fail?

So I think this is because we don't check for coreMigrationVersion in the outdatedDocumentsQuery. So unless:
a) there's incompatible mappings and we reindex all documents
b) all saved object types have a new typeMigrationVersion (unlikely)

Some documents will be the old coreMigrationVersion and require the migrationVersion mapping.

pgayvallet

Overall great job, this was some massive work.

Regarding the HTTP APIs, even if I personally think that it would probably be fine for 99% of the usages, I'm forced to agree with @rudolf on the fact that we should probably be cautious, and try to preserve the existing shape for responses when possible.

My other comments and remarks:

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/internal_utils.ts

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts

pgayvallet · 2023-03-22T12:22:49Z

...s/core/saved-objects/core-saved-objects-base-server-internal/src/serialization/serializer.ts

      ...(migrationVersion && { migrationVersion }),
      ...(coreMigrationVersion && { coreMigrationVersion }),
+      ...(typeMigrationVersion != null ? { typeMigrationVersion } : {}),


NIT: There's a subtle difference of behavior between typeMigrationVersion and the other attributes here (X && { X } vs X != null ? { X } : {}) . Not saying that one is better than the other, but isn't that a risk somehow to have this specific prop handled differently here?

That's good notice. That's done so to keep backward compatibility -- previously, we used migrationVersion: {} to trigger migrations, but now an empty string does that trick. In order to keep the value, we are using this ternary operator.

...cts/core-saved-objects-import-export-server-internal/src/import/lib/collect_saved_objects.ts

...core/saved-objects/core-saved-objects-migration-server-internal/src/core/migrate_raw_docs.ts

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

pgayvallet · 2023-03-22T13:01:43Z

...-saved-objects-migration-server-internal/src/document_migrator/document_migrator_pipeline.ts

+    private convertNamespaceTypes: boolean
+  ) {}
+
+  protected *getPipeline(): Generator<Transform> {


I was going to say that I wasn't sure a generator was really necessary given that the migration workstream is predictable, but actually because of the edge case of type switch, it's not 😄

As a side note for later me/us, we will need to break/remove that type switching capability for ZDT

pgayvallet · 2023-03-22T13:04:25Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/initial_state.ts

+          must: [
+            { term: { type } },
+            {
+              bool: {
+                should: [
+                  {
+                    bool: {
+                      must: { exists: { field: 'migrationVersion' } },
+                      must_not: { term: { [`migrationVersion.${type}`]: latestVersion } },
+                    },


Backlinking #153031, because the equivalent query for the zdt algorithm will have to be adapted too.

pgayvallet · 2023-03-22T13:30:47Z

src/core/server/integration_tests/saved_objects/migrations/group2/batch_size_bytes.test.ts

@@ -118,7 +118,7 @@ describe('migration v2', () => {
    await root.preboot();
    await root.setup();
    await expect(root.start()).rejects.toMatchInlineSnapshot(
-      `[Error: Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715272 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.]`
+      `[Error: Unable to complete saved object migrations for the [.kibana] index: The document with _id "canvas-workpad-template:workpad-template-061d7868-2b4e-4dc8-8bf7-3772b52926e5" is 1715248 bytes which exceeds the configured maximum batch size of 1015275 bytes. To proceed, please increase the 'migrations.maxBatchSizeBytes' Kibana configuration option and ensure that the Elasticsearch 'http.max_content_length' configuration option is set to an equal or larger value.]`


NIT: Outside of the scope of this PR but we should probably adapt those tests to not assert on the exact size of the documents 🙈

pgayvallet · 2023-03-22T14:10:53Z

...saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts

+      typeMigrationVersion: {
+        type: 'version',
+      },


Question: given we will be converting it to typeMigrationVersion during the migration, do we need to keep the migrationVersion field in our schema?

Yes, we still need it to provide compatible mappings.

kibana-ci · 2023-03-23T04:31:39Z

💚 Build Succeeded

Buildkite Build
Commit: c2c6c1f

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-server`	98	99	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	351.1KB	351.3KB	+182.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id	before	after	diff
`typeMigrationVersion`	-	1	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-api-browser`	107	109	+2
`@kbn/core-saved-objects-api-server`	334	338	+4
`@kbn/core-saved-objects-server`	489	491	+2
total			+8

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

References to deprecated APIs

id	before	after	diff
`@kbn/core-saved-objects-api-browser`	54	60	+6
`@kbn/core-saved-objects-api-server-internal`	0	8	+8
`@kbn/core-saved-objects-browser-internal`	238	251	+13
`@kbn/core-saved-objects-browser-mocks`	32	35	+3
`@kbn/core-saved-objects-import-export-server-internal`	56	58	+2
`@kbn/core-saved-objects-server-internal`	4	6	+2
`canvas`	151	152	+1
`fleet`	51	62	+11
`graph`	82	88	+6
`lists`	93	95	+2
`securitySolution`	361	415	+54
total			+108

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💚 Build #114012 succeeded 20efde54bb431632b831bc8f8b4f1cd0457ead69
💚 Build #113233 succeeded 89301b21739c7d900be770c7cdf78923d1586e26
💚 Build #113159 succeeded 4e3aa3a14562558412d27df94ae760cae265b819
💔 Build #113139 failed 1a869a731690a4941024591626a53d489e3ae3dc
💚 Build #112922 succeeded 4edf0e1d11ba8bd173c834bc9122dc06ab78c2b4
💔 Build #112873 failed 6b69a097668cc5a70b69496bd69a70aab4d0d4a6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

pgayvallet

LGTM

In addition to the endpoints BWC, we probably want to open a follow-up issue to talk about whether we should change the migration algorithm(s) to forcefully migrate all documents when coreMigrationVersion changes.

rudolf

I will create an issue for fixing API compatibility and migrationVersion mappings in a separate PR

…n string value (elastic#150075) * Update document migrator to rely on `typeMigrationVersion` instead of `migrationVersion`. * Refactor document migrator to extract migration pipeline logic. * Add `core` migration type.

dokmic added 2 commits January 25, 2023 12:02

Update saved objects migrator to handle the migrationVersion as a s…

a0603f2

…tring value

Update saved objects core packages to handle the migrationVersion a…

9f88063

…s a string value

dokmic changed the title ~~[Saved Objects]~~ [Saved Objects] Update the migrationVersion property to hold a plain string value Feb 1, 2023

dokmic mentioned this pull request Feb 10, 2023

[Saved Objects] Update document migrator not to bump core version on every migration #150443

Merged

1 task

dokmic added 2 commits February 13, 2023 16:34

Extract the document migrator pipeline into a separate entity

c2cf9f0

Add a separate core transform type to handle schema migrations

9191797

dokmic force-pushed the feature/70815 branch 3 times, most recently from 91bd29b to 68ddd12 Compare February 20, 2023 11:08

dokmic added 5 commits February 22, 2023 17:02

Provide backward compatibility in the Saved Objects API

b38c9e1

Revert changing the migrationVersion type to string

fd7b6a9

Add an additional typeMigrationVersion property deprecating `migrat…

58f1d38

…ionVersion`

Update document migrator to rely on typeMigrationVersion property i…

da2cc1b

…nstead of deprecated `migrationVersion`

Update tests

4adee70

dokmic force-pushed the feature/70815 branch from 68ddd12 to 45c34f6 Compare February 27, 2023 15:56

dokmic force-pushed the feature/70815 branch 3 times, most recently from 673b578 to cb5b1e4 Compare March 8, 2023 10:18

dokmic added 2 commits March 8, 2023 15:06

Update outdated documents query to take into account migrated `typeMi…

e4d5bf5

…grationVersion` property

Update sample data sets to use updated migration versions format

346858a

dokmic force-pushed the feature/70815 branch 4 times, most recently from 4e3aa3a to 89301b2 Compare March 10, 2023 08:26

dokmic force-pushed the feature/70815 branch from 89301b2 to 20efde5 Compare March 20, 2023 12:56

dokmic marked this pull request as ready for review March 20, 2023 12:56

dokmic requested review from a team as code owners March 20, 2023 12:56

dokmic added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes Feature:Migrations v8.8.0 labels Mar 20, 2023

dokmic added the backport:skip This commit does not require backporting label Mar 20, 2023

dokmic requested review from rudolf and pgayvallet March 20, 2023 12:58

vadimkibana approved these changes Mar 20, 2023

View reviewed changes

nreese approved these changes Mar 20, 2023

View reviewed changes

jgowdyelastic approved these changes Mar 21, 2023

View reviewed changes

rudolf requested changes Mar 22, 2023

View reviewed changes

pgayvallet reviewed Mar 22, 2023

View reviewed changes

Refactor to align with the rest of the code

c2c6c1f

dokmic force-pushed the feature/70815 branch from 20efde5 to c2c6c1f Compare March 23, 2023 03:26

pgayvallet mentioned this pull request Mar 23, 2023

Implement the second part of the ZDT migration algorithm #153031

Merged

6 tasks

pgayvallet requested a review from rudolf March 23, 2023 12:22

pgayvallet approved these changes Mar 23, 2023

View reviewed changes

rudolf approved these changes Mar 24, 2023

View reviewed changes

rudolf mentioned this pull request Mar 24, 2023

Changing migrationVersion -> typeMigrationVersion in schema should not introduce breaking changes to SO APIs #153644

Closed

dokmic merged commit 17876df into elastic:main Mar 24, 2023

dokmic deleted the feature/70815 branch March 24, 2023 12:46

rudolf mentioned this pull request Apr 3, 2023

Remove migrationVersion mappings in 8.8.0 #154246

Closed

dokmic mentioned this pull request May 11, 2023

[Migrations] Outdated documents query matches all the documents in the temp index #157354

Closed

rudolf mentioned this pull request Aug 24, 2023

[EPM] Kubernetes integration fails to install #164690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Objects] Update the `migrationVersion` property to hold a plain string value #150075

[Saved Objects] Update the `migrationVersion` property to hold a plain string value #150075

dokmic commented Feb 1, 2023 •

edited

Loading

elasticmachine commented Mar 20, 2023

vadimkibana left a comment

nreese left a comment

jgowdyelastic left a comment

rudolf left a comment

rudolf Mar 22, 2023

dokmic Mar 23, 2023

rudolf Mar 23, 2023

rudolf Mar 24, 2023

pgayvallet left a comment

pgayvallet Mar 22, 2023

dokmic Mar 23, 2023

pgayvallet Mar 22, 2023

pgayvallet Mar 22, 2023

pgayvallet Mar 22, 2023

pgayvallet Mar 22, 2023

dokmic Mar 23, 2023

kibana-ci commented Mar 23, 2023

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

pgayvallet left a comment

rudolf left a comment

[Saved Objects] Update the migrationVersion property to hold a plain string value #150075

[Saved Objects] Update the migrationVersion property to hold a plain string value #150075

Conversation

dokmic commented Feb 1, 2023 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Mar 20, 2023

vadimkibana left a comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Mar 23, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

Saved Objects .kibana field count

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

pgayvallet left a comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

[Saved Objects] Update the `migrationVersion` property to hold a plain string value #150075

[Saved Objects] Update the `migrationVersion` property to hold a plain string value #150075

dokmic commented Feb 1, 2023 •

edited

Loading